-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(fee-breakdown): Initial setup and logic with useFeeBreakdown()
hook
#4010
Conversation
Removed vultr server and associated DNS entries |
ecc119b
to
595d8dd
Compare
applicationFee: | ||
data["application.fee.calculated"] || data["application.fee.payable"], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This essentially matches the logic here -
const schema = z | ||
.object({ | ||
"application.fee.calculated": feeSchema.optional().default(0), | ||
"application.fee.payable": feeSchema, | ||
"application.fee.payable.vat": feeSchema.optional().default(0), | ||
}) | ||
.transform(toFeeBreakdown); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This parse & transform pattern might be a nice approach to try in planx-core
passport → payload mappings.
ec21eb7
to
5569c90
Compare
5569c90
to
bd6ad07
Compare
useFeeBreakdown()
hook
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks overall good - a few clarifying questions and language suggestions but nothing stopping merge at this point as this is all feature-flagged 👍
@@ -0,0 +1,12 @@ | |||
export interface FeeBreakdown { | |||
applicationFee: number; | |||
total: number; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: introducing applicationFee
and total
language feels like a bit of extra mental load here and I wonder if this can re-use calculated
(which if undefined, fallsback to payable) and payable
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before reading the tests, I would have also personally guessed that total
= full calculated
amount and applicationFee
= payable
but your passport mappings are the other way around !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Useful feedback thanks - this was an intentional decision to try and make it a little clearer and more explicit (I did also consider gross/net as well at one point).
Clearly not successful though 😂 Your point about consistency and mental load is a very valid one 👍
Thanks very much for the review 👍 Due to branches and other PRs which rely on this I'm going to fix these issues forward - all currently feature flagged. |
What does this PR do?
Sets up basic logic for parsing fee details from the passport, and displaying them to the user via the
FeeBreakdown
componentDesign decisions
These are largely points up for discussion, I just want to explain some of my reasoning and outline potential next steps.
If we cannot parse values from the passport, do not show the breakdown to the user
Even if the
showFeeBreakdown
prop is set totrue
, the breakdown still won't show if we cannot display it to the user (e.g. missing or invalid values). If this happens it's a content issue which will be flagged via Airbrake for content editors to ultimately fix - the user journey won't be disrupted.Hardcode
application.fee
data fieldThis is a rough MVP decision just to move forward initially, it's not a big additional task to read
data.fn
from the relevant Pay component here.Reductions and exemptions are combined to a generic "reduction" value
The fee calculator doesn't associate a numerical value with exemptions and reductions, but just updates
application.fee.payable
and sets true/false values. There's a few options around how we could handle this -application.fee.payable
I'm very keen to avoid repeating any logic already handled in the
fee-calculator
flow - doing so puts us in an awkward code vs. low-code spot with regards to how this logic is handled.Again, I think we take a fallback approach here. If we cannot parse a list of exemptions or reductions from the passport, we can fallback to summed generic "reductions" amount?
VAT is set via a Calculate component, and not re-calculated
I'm not sure about this one - it allows room for error if an Editor incorrectly calculates this value. A more robust approach might be to allow the user to set a
application.fee.payable.includesVAT
variable, and display an amount based on this.useFeeBreakdown()
#4042Nothing is really explained to Editors
I think this will need something - I think just adding a list of variables and how this words in the Editor would achieve this.
Disabled for Invite to pay journeys
This is something I hadn't initially considered - we have no passport when returning to the service via invite to pay so the current approach does not work. We'll need to persist the fee breakdown data to the
payment_requests
table in a similar way to howgovpay_metadata
is stored. I'll open another PR shortly to provide a solution here.Next steps...
Here's what I'd like to go for as I suspect the current implementation is too MVP -
showFeeBreakdown
is checked